send warning when we receive a old commitment transaction #1430

vincenzopalazzo · 2022-04-18T21:52:53Z

During a channel_reestablish now we send a warning message when we receive an old commitment transaction.

Fixes #1207 with the following consideration lightning/bolts#934 (comment)

lightning/src/ln/channel.rs

codecov-commenter · 2022-04-22T11:10:52Z

Codecov Report

Merging #1430 (e6300da) into main (171dfee) will increase coverage by 0.46%.
The diff coverage is 88.57%.

@@            Coverage Diff             @@
##             main    #1430      +/-   ##
==========================================
+ Coverage   90.89%   91.36%   +0.46%     
==========================================
  Files          75       75              
  Lines       41657    43716    +2059     
  Branches    41657    43716    +2059     
==========================================
+ Hits        37866    39941    +2075     
+ Misses       3791     3775      -16

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.57% <75.00%> (+4.18%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.40% <92.59%> (+0.22%)`	⬆️
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/util/config.rs	`48.35% <0.00%> (+3.45%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171dfee...e6300da. Read the comment docs.

lightning/src/ln/functional_tests.rs

lightning/src/ln/channel.rs

lightning/src/ln/functional_tests.rs

vincenzopalazzo

OK! I tried to make a little bit of refactoring and applied the changes that you suggested.

The only think that is unclear to me right now, is this case

https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channel.rs#L3708-L3712

Why in this case there is a CloseDelayBroadcast why does mean? i was not able to find some comments about the meaning, and I didn't check if the spec has some info about this case!

TheBlueMatt

Sorry for the delay in reviewing this!

lightning/src/ln/functional_tests.rs

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/functional_test_utils.rs

vincenzopalazzo · 2022-04-27T20:10:26Z

Rebased on master, I reorganized the test in a more clean way where the different cases are executed at the end of the function. In this way, it is cleaner to the reader what we are trying to do!

TheBlueMatt

Awesome! One note - we don't like having intermediate commits that fail tests. We don't test for it in CI (only that it compiles), but if at all possible try to make sure each individual commit passes tests as well, ensuring we can git bisect easily.

lightning/src/ln/functional_test_utils.rs

vincenzopalazzo · 2022-04-28T15:49:56Z

we don't like having intermediate commits that fail tests. We don't test for it in CI (only that it compiles), but if at all possible try to make sure each individual commit passes tests as well, ensuring we can git bisect easily.

Sorry! I rebased and squash the commit in a single one!

TheBlueMatt

Basically looks good, one comment on the second branch of the test.

lightning/src/ln/channel.rs

lightning/src/ln/functional_tests.rs

TheBlueMatt

LGTM!

lightning/src/ln/functional_tests.rs

TheBlueMatt · 2022-05-03T15:46:48Z

Hmm, was there a conflict? In general please avoid rebase'ing unless there's a conflict with the current upstream git.

TheBlueMatt

Basically LGTM, lets get another reviewer.

lightning/src/ln/functional_tests.rs

lightning/src/ln/channel.rs

lightning/src/ln/functional_tests.rs

vincenzopalazzo · 2022-05-03T20:46:09Z

I clean up the code and remove the enum. thanks, @valentinewallace to point me on this, and also for the grammar mistake!

I also rebased on top of the last master!

TheBlueMatt · 2022-05-03T21:04:28Z

I also rebased on top of the last master!

Generally please avoid this unless there is a conflict. Rebases make it harder to diff-tree and see changes to a PR, range-diff can be a bit more finicky and hard to read.

vincenzopalazzo · 2022-05-03T22:09:54Z

Ops! sorry @TheBlueMatt

lightning/src/ln/functional_tests.rs

lightning/src/ln/channel.rs

During a `channel_reestablish` now we send a warning message when we receive a old commitment transaction from the peer. In addition, this commit include the update of functional test to make sure that the receiver will generate warn messages. Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from f40c9bb to 136171d Compare April 18, 2022 21:59

TheBlueMatt reviewed Apr 19, 2022

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch 5 times, most recently from 7ae6fdc to fc33225 Compare April 22, 2022 10:15

vincenzopalazzo changed the title ~~send warning when we have a old commitment transaction~~ send warning when we receive a old commitment transaction Apr 22, 2022

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch 2 times, most recently from c70e6d3 to d04ca27 Compare April 22, 2022 11:10

vincenzopalazzo marked this pull request as ready for review April 22, 2022 11:51

vincenzopalazzo commented Apr 22, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

vincenzopalazzo requested a review from TheBlueMatt April 22, 2022 12:00

TheBlueMatt reviewed Apr 22, 2022

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 5ada132 to 3e41b90 Compare April 22, 2022 17:22

vincenzopalazzo commented Apr 22, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

vincenzopalazzo commented Apr 22, 2022

View reviewed changes

vincenzopalazzo requested a review from TheBlueMatt April 22, 2022 20:03

TheBlueMatt reviewed Apr 25, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from c80ea92 to 5a55542 Compare April 25, 2022 19:45

TheBlueMatt reviewed Apr 26, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 5a55542 to 43edafa Compare April 27, 2022 20:05

TheBlueMatt reviewed Apr 28, 2022

View reviewed changes

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch 2 times, most recently from 53d7cda to 8931516 Compare April 28, 2022 15:49

TheBlueMatt reviewed Apr 28, 2022

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 8931516 to 6dac37b Compare April 28, 2022 20:55

vincenzopalazzo mentioned this pull request Apr 29, 2022

Nodes shouldn't publish their commitment when receiving outdated channel_reestablish lightning/bolts#934

Open

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 6dac37b to c341020 Compare May 1, 2022 11:15

vincenzopalazzo requested a review from TheBlueMatt May 1, 2022 11:16

TheBlueMatt reviewed May 2, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch 2 times, most recently from d3651e1 to 833a74e Compare May 3, 2022 12:58

TheBlueMatt reviewed May 3, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

TheBlueMatt added the Seeking Code Review label May 3, 2022

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 833a74e to ae47520 Compare May 3, 2022 17:03

valentinewallace self-requested a review May 3, 2022 17:17

valentinewallace reviewed May 3, 2022

View reviewed changes

valentinewallace removed the Seeking Code Review label May 3, 2022

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from ae47520 to 5dd4662 Compare May 3, 2022 20:45

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 5dd4662 to a399d1f Compare May 3, 2022 22:14

valentinewallace reviewed May 3, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch 2 times, most recently from 6ecb4d6 to 040b3cd Compare May 3, 2022 23:01

TheBlueMatt reviewed May 3, 2022

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

valentinewallace reviewed May 4, 2022

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

vincenzopalazzo force-pushed the macros/channel_reestablish_v2 branch from 040b3cd to e6300da Compare May 4, 2022 07:23

valentinewallace approved these changes May 4, 2022

View reviewed changes

TheBlueMatt approved these changes May 4, 2022

View reviewed changes

TheBlueMatt merged commit 9fbafd4 into lightningdevkit:main May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send warning when we receive a old commitment transaction #1430

send warning when we receive a old commitment transaction #1430

vincenzopalazzo commented Apr 18, 2022 •

edited

Loading

codecov-commenter commented Apr 22, 2022 •

edited

Loading

vincenzopalazzo left a comment

TheBlueMatt left a comment

vincenzopalazzo commented Apr 27, 2022

TheBlueMatt left a comment

vincenzopalazzo commented Apr 28, 2022

TheBlueMatt left a comment

TheBlueMatt left a comment

TheBlueMatt commented May 3, 2022

TheBlueMatt left a comment

vincenzopalazzo commented May 3, 2022

TheBlueMatt commented May 3, 2022

vincenzopalazzo commented May 3, 2022

send warning when we receive a old commitment transaction #1430

send warning when we receive a old commitment transaction #1430

Conversation

vincenzopalazzo commented Apr 18, 2022 • edited Loading

codecov-commenter commented Apr 22, 2022 • edited Loading

Codecov Report

vincenzopalazzo left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

vincenzopalazzo commented Apr 27, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

vincenzopalazzo commented Apr 28, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 3, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

vincenzopalazzo commented May 3, 2022

TheBlueMatt commented May 3, 2022

vincenzopalazzo commented May 3, 2022

vincenzopalazzo commented Apr 18, 2022 •

edited

Loading

codecov-commenter commented Apr 22, 2022 •

edited

Loading